-
-
Notifications
You must be signed in to change notification settings - Fork 43
kinds: rm unused kind K"inert" #561
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Yes, I think this is fine, assuming the implementation of quasi-quote knows how to handle the implicit quoting in |
|
👍 I'm assuming JuliaLowering must understand this and for flisp lowering, |
|
Looks fine to me, though I hope for a day these can be in Base |
Isn't |
|
Or were you saying you want syntax for it? |
|
Just syntax, and not wanting to juggle code between JuliaLowering and JuliaSyntax |
`inert` is mostly used in lowering. As far as I can tell, there is precisely one place in the flisp parser that produces it: https://github.com/JuliaLang/julia/blob/d6b3669621ceb18aea693d8544b2c38870d289ad/src/julia-parser.scm#L1259 However, we do not insert and `inert` there in JuliaSyntax, instead preferring to automatically quote the rhs of a `.` head in expr conversion: https://github.com/JuliaLang/JuliaSyntax.jl/blob/eceaa39706c6ecc3ef496613096216625dba558e/src/expr.jl#L310 As a result, (and as code coverage complains), the syntax head is unused and should be removed here. However, it should probably then be added back in JuliaLowering.
|
We might want to revert this. I didn't know at the time that JuliaLowering uses expr.jl for (pre-lowering) SyntaxTree->Expr conversion. K"inert" won't be in the tree if we got it from normal parsing, but if it's been through macro expansion (or is from expr->syntaxtree, depending on whether we end up back-converting QuoteNodes) it can show up. |
|
It feels like we should possibly introduce some proper layering here then so that JuliaLowering can add additional forms - it seems weird that the conversion logic for such forms should go into the parser. Unless we think that |
|
Maybe. On the other hand, I'll probably be setting up a JuliaLowering layer to cc @c42f in case you have opinions |
|
Thanks for the mention @mlechu. I didn't see this go past when it was proposed and I was out of action. Personally I feel like Anyway, so it's probably not very harmful to have removed The issue of dealing with hygiene is separate - as you say that's definitely a lowering problem and shouldn't intrude on JuliaSyntax stuff so we do need a way to deal with that and I'm not 100% sure what is cleanest but I'm thinking about it. |
`inert` is mostly used in lowering. As far as I can tell, there is precisely one place in the flisp parser that produces it: https://github.com/JuliaLang/julia/blob/d6b3669621ceb18aea693d8544b2c38870d289ad/src/julia-parser.scm#L1259 However, we do not insert and `inert` there in JuliaSyntax, instead preferring to automatically quote the rhs of a `.` head in expr conversion: https://github.com/JuliaLang/JuliaSyntax.jl/blob/5f64351f4546c0e782760eb9e835fac89644bd44/src/expr.jl#L310 As a result, (and as code coverage complains), the syntax head is unused and should be removed here. However, it should probably then be added back in JuliaLowering.
inertis mostly used in lowering. As far as I can tell, there is precisely one place in the flisp parser that produces it:https://github.com/JuliaLang/julia/blob/d6b3669621ceb18aea693d8544b2c38870d289ad/src/julia-parser.scm#L1259
However, we do not insert and
inertthere in JuliaSyntax, instead preferring to automatically quote the rhs of a.head in expr conversion:JuliaSyntax.jl/src/expr.jl
Line 310 in eceaa39
As a result, (and as code coverage complains), the syntax head is unused and should be removed here. However, it should probably then be added back in JuliaLowering.